Correct type annotations on NetworkX DiGraphs#14595
Conversation
|
Thanks, this closes #14592 |
|
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
- discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
+ discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
|
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | ||
| @overload | ||
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | ||
| def data(self, data: bool | str = True, default=None) -> NodeDataView[_Node]: ... | ||
| def __call__(self, data: Literal[True] | str, default=None) -> Self: ... | ||
| def data(self, data: bool | str = True, default=None) -> Self: ... |
There was a problem hiding this comment.
I believe @srittau 's comment in #14595 (comment) only applied to the first overload
When data is True, it's not returning self
As-is, this overload is useless since only the params differ (could be a Union, but I think it was correct to have an overload, see:)
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | |
| def data(self, data: bool | str = True, default=None) -> NodeDataView[_Node]: ... | |
| def __call__(self, data: Literal[True] | str, default=None) -> Self: ... | |
| def data(self, data: bool | str = True, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | |
| @overload | |
| def data(self, data: Literal[False], default=None) -> Self: ... | |
| @overload | |
| def data(self, data: Literal[True] | str = True, default=None) -> NodeDataView[_Node]: ... |
There was a problem hiding this comment.
You are right; I misread NodeView as NodeDataView in a haste.
| @overload | ||
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> int: ... # type: ignore[overload-overlap] | ||
| @overload | ||
| def __call__(self, nbunch: None | Iterable[_Node], weight: None | bool | str = None) -> Self: ... |
There was a problem hiding this comment.
The overlap error is correct here. I also think it's inverted since nbunch=None should return self
| @overload | |
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> int: ... # type: ignore[overload-overlap] | |
| @overload | |
| def __call__(self, nbunch: None | Iterable[_Node], weight: None | bool | str = None) -> Self: ... | |
| @overload | |
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> Self: ... | |
| @overload | |
| def __call__(self, nbunch: Iterable[_Node], weight: None | bool | str = None) -> int: ... |
There was a problem hiding this comment.
You're right; the overload should be specialized to _Node (which still needs to ignore possible overlap between None/NoneType and _Node, as is done in OutEdgeView).
None and Iterable[_Node] return Self (first and last return), but _Node returns int (middle two returns)
(ignoring, like we have been, that it could actually return float in the case of graphs whose edge weights are float)
|
Oh I was too slow on the review and this got merged ^^" No worries, I'll propose the same changes in #14597 |
4 participants
Two fixes:
in_degreeis acached_propertythat isInDegreeVieworInMultiDegreeView(for aMultiDiGraph).InDegreeViewcan then return anintor itself when called. However, theGraph's propertyin_degreeis never itself anint, always anInDegreeView(source).NodeViewreturns not just anyIterator, but aNodeViewitself. See example in documentation,